Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integration test for when remote actor username changes #30361

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

angusmcleod
Copy link

@renchap renchap added the activitypub Protocol-related changes, federation label May 18, 2024
@ClearlyClaire
Copy link
Contributor

Hi! Thank you for your PR!
Mastodon doesn't officially support renaming, but as you've seen, it should still handle things relatively gracefully (even though there's still room for improvement).

At a glance, the PR looks ok, but I will do a full review later.

It's still marked as a draft, so is there something in particular you want feedback on?

@angusmcleod angusmcleod marked this pull request as ready for review May 23, 2024 14:19
@angusmcleod
Copy link
Author

@ClearlyClaire Thanks for taking a look, it's much appreciated. I've added a few more tests for this case, so it's ready for review now.

@angusmcleod
Copy link
Author

angusmcleod commented May 23, 2024

@ClearlyClaire Thanks for the feedback. This is what I've done:

  • Made the formatting changes you suggested
  • Removed the Update Note and Follow specs as suggested
  • Added update username support for Update Actor. I think it makes more sense to directly update the username in this case instead of performing a migration, as it's a contained case (i.e. nothing else is going on).
  • Added more tests for the Search case (it occurred to me that some services may still return the actor for the old handle).

@ClearlyClaire
Copy link
Contributor

  • Added update username support for Update Actor. I think it makes more sense to directly update the username in this case instead of performing a migration, as it's a contained case (i.e. nothing else is going on).

I think we should do something like this eventually, but this is a much more complex endeavour. Indeed, we enforce uniqueness of (username, domain) tuples, so this introduces a new failure path (another account with the same username already exists) that needs extra consideration.

@angusmcleod
Copy link
Author

angusmcleod commented May 24, 2024

@ClearlyClaire Ensuring username uniqueness is something the remote domain has responsibility for in this case. We can protect against that within the context of Update Actor: angusmcleod@138fee1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants